send: return error for v1 URIs in v2 sender - #1870
Conversation
798471f to
990ca19
Compare
Coverage Report for CI Build 34412200999Coverage increased (+0.02%) to 86.663%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Hi @vbrekher , thanks for picking this up. The added type |
|
Good catch, thanks. I gated |
|
@vbrekher, logs show another unrelated error; you'll need to update the JS assertion too |
|
Thanks, found it in the JS integration log. The v1 URI now fails at |
And you'll actually want |
d94e568 to
ec30fce
Compare
Signed-off-by: v ₿ <valentin.brekher@gmail.com>
ec30fce to
34cc3a4
Compare
|
Yep, fixed. The gate is now |
|
|
xstoicunicornx
left a comment
There was a problem hiding this comment.
There is small formatting CI error that needs to be addressed.
This would also be a breaking change to interface now that we are on v1.0.0, not sure what the protocol is for handling/documenting that.
One other piece of feedback below but otherwise looks good overall.
| new payjoin.SenderBuilder(psbt, pjUri).buildRecommended( | ||
| 18446744073709551615n, | ||
| ); | ||
| }, /RuntimeError/); | ||
| new payjoin.SenderBuilder(psbt, pjUri); | ||
| }, /SenderInputError\.Build/); |
There was a problem hiding this comment.
This test was for oversized fee rate in SenderBuilder::build_recommended. We should probably update this test to use v2 URI instead to avoid test regression.
Maybe we want to add tests for handling the v1 URI error for all the bindings as well?
SenderBuilder::newin the v2 sender currently panics when it receives a v1 Payjoin URI.Make the constructor fallible instead and return a normal build error for unsupported v1 URIs. The FFI constructor propagates the same failure through
SenderInputError::Build.This keeps the v1 sender behavior unchanged while making v2 safe to use with untrusted Payjoin URIs.
AI assistance
ChatGPT was used to inspect the codebase, develop the patch, add tests, run validation, and draft this PR description.
Closes #1849